Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement notify-only support #59

Merged
merged 16 commits into from
Jan 13, 2021
Merged

Implement notify-only support #59

merged 16 commits into from
Jan 13, 2021

Conversation

jantman
Copy link
Contributor

@jantman jantman commented Jan 11, 2021

Description

This PR implements notify_only support for policies, which can also be used in defaults.yml to put an entire account into notify-only mode. This is controlled by a notify_only boolean top-level policy key, which policygen then handles and strips out when generating the final custodian YAML files.

The current functionality of notify-only is as follows:

  • The comment / comments / description fields will be prefixed with the string NOTIFY ONLY:
  • If the policy has a tags list, a notify-only tag will be appended to it.
  • All tagging actions will have the string -notify-only appended to their tag names, to automate the above-described transition. Specifically:
    • Any mark or tag actions in the actions list will have the string -notify-only appended to their tag or key values (if present) or appended to every item in their tags list (if present). If none of the above are present, the tag item will be set to custodian's DEFAULT_TAG value, with -notify-only appended.
    • Any mark-for-op actions will have the string -notify-only appended to their tag value. If they do not already have a tag value, it will be set to custodian's DEFAULT_TAG value, with -notify-only appended.
    • Any remove-tag / unmark / untag actions wukk have the string -notify-only appended to all items in their tags list.
  • All notify actions will have their violation_desc, if present, prefixed with NOTIFY ONLY:. Their action_desc, if present, will be prefixed with in the future (currently notify-only).
  • All other action types, not listed above, will be removed from the policy. We enforce notify-only by only retaining specifically whitelisted actions in the policy.

Testing Done

  1. Complete unit test coverage.
  2. Ran a dry-run internally on one of our accounts. Ping me on Slack, or see the card for this, for a link to the diff.

@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #59 (073983a) into master (52847cd) will increase coverage by 2.67%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   57.98%   60.65%   +2.67%     
==========================================
  Files           8        9       +1     
  Lines        1397     1492      +95     
  Branches      264      294      +30     
==========================================
+ Hits          810      905      +95     
  Misses        586      586              
  Partials        1        1              
Impacted Files Coverage Δ
manheim_c7n_tools/notifyonly.py 100.00% <100.00%> (ø)
manheim_c7n_tools/policygen.py 99.76% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52847cd...073983a. Read the comment docs.

@jantman jantman merged commit 28c6e34 into manheim:master Jan 13, 2021
@jantman jantman deleted the notify-only branch January 13, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants